Skip to content

REST API: Migrate site settings endpoints #8638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Jan 16, 2023
Merged

REST API: Migrate site settings endpoints #8638

merged 10 commits into from
Jan 16, 2023

Conversation

@itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Jan 16, 2023 •
edited
Loading

Closes: #8637

Description

This PR updates the site settings mapper and enables REST API for site settings endpoints.

Testing instructions

Pre-requisite: due to a rate-limit issue with Pressable sites, create a JN site to ensure that testing is smooth for now.

  • Enable feature flag applicationPasswordAuthenticationForSiteCredentialLogin and build the app.
  • Log out of the app or skip onboarding if needed.
  • On the prologue screen, select Enter your site address and enter the address of your self-hosted site.
  • Proceed to log in with site credentials.
  • When the login succeeds, you should see the home screen.
  • You should then see in Xcode console: 🎛 Site settings sync completed for siteID and not ⛔️ Site settings sync had <count> error(s) for siteID.

Testing setting loading and updating.

  • In WP Admin, select WooCommerce > Settings > General and disable the option "Enable the use of coupon codes ".
  • On the app, enable coupons if needed (in Settings > Experimental features > Coupon management).
  • Navigate to the Menu app and select Coupons. Notice that the screen shows a message saying coupons are disabled for the store.
  • Select Enable coupon. Notice that the list will the be reloaded and show the correct data after that.
  • Reload the General WC settings page on WP Admin, the setting for coupons should be enabled again.

Screenshots

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-16.at.12.54.26.mp4
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@itsmeichigo itsmeichigo added type: task An internally driven task. feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. labels Jan 16, 2023
@itsmeichigo itsmeichigo added this to the 12.0 milestone Jan 16, 2023
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 16, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8638-419896c on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@itsmeichigo itsmeichigo marked this pull request as ready for review January 16, 2023 05:59
Base automatically changed from feat/8602-system-status-migration to trunk January 16, 2023 06:34
@jaclync jaclync self-assigned this Jan 16, 2023
jaclync
jaclync approved these changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected 👍 just had a question about the way it checks if it should skip fetching site plan

WooCommerce/Classes/Yosemite/DefaultStoresManager.swift
let sitePlanAction = AccountAction.synchronizeSitePlan(siteID: siteID) { result in
if case let .failure(error) = result {
errors.append(error)
/// skips synchronizing site plan if logged in with WPOrg credentials
Copy link
Contributor

@jaclync jaclync Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be worth mentioning why? is it because the site plan is for WPCOM users only?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the site plan API is a Dotcom endpoint and we cannot handle this request for WPOrg-authenticated users. I updated the comment in 419896c.

WooCommerce/Classes/Yosemite/DefaultStoresManager.swift Outdated
if case let .failure(error) = result {
errors.append(error)
/// skips synchronizing site plan if logged in with WPOrg credentials
if siteID != WooConstants.placeholderStoreID {
Copy link
Contributor

@jaclync jaclync Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to check this with isAuthenticatedWithoutWPCom?

woocommerce-ios/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift

Line 66 in 26893e9

var isAuthenticatedWithoutWPCom: Bool {

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - also updated in 419896c.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 16, 2023

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@itsmeichigo itsmeichigo enabled auto-merge January 16, 2023 07:38
@itsmeichigo itsmeichigo merged commit 602cd36 into trunk Jan 16, 2023
@itsmeichigo itsmeichigo deleted the feat/8637-site-settings-migration branch January 16, 2023 07:53
@itsmeichigo itsmeichigo mentioned this pull request Jun 8, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaclync jaclync jaclync approved these changes

@selanthiraiyan selanthiraiyan Awaiting requested review from selanthiraiyan

Assignees

@jaclync jaclync

Labels

feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. type: task An internally driven task.

Projects

None yet

Milestone

12.0

Development

Successfully merging this pull request may close these issues.

REST API: Migrate site settings endpoints

4 participants

@itsmeichigo @wpmobilebot @jaclync